Split runfile tests up to improve helix usage#53452
Split runfile tests up to improve helix usage#53452marcpopMSFT wants to merge 7 commits intomainfrom
Conversation
RunFileTests was a single 6,210-line class with ~150 IL methods. The AssemblyScheduler (methodLimit=16) cannot split within a class, so the entire class landed in one Helix shard (shard 19) taking ~42 minutes while all other dotnet.Tests.dll shards completed in under 8 minutes. Split into 5 test classes inheriting from a shared RunFileTestBase: - RunFileTests (31 tests): path resolution, precedence, stdin, multifile - RunFileTests_BuildOptions (28 tests): working dir, Dir.Build.props, arguments, binary log, verbosity, embedded resources - RunFileTests_BuildCommands (28 tests): restore, build, publish, pack, clean, launch profiles - RunFileTests_Directives (24 tests): defines, package/SDK/project refs, include directive, user secrets, CSC arguments - RunFileTests_CscOnlyAndApi (42 tests): up-to-date checks, csc-only mode, API tests, entry points, MSBuild get RunFileTestBase holds shared static fields (changed from private to internal), helper methods (DirectiveError, VerifyBinLogEvaluationDataCount), and the Build() instance method used across multiple test classes. Total test count preserved: 153 methods (123 Fact + 27 Theory + 3 PlatformSpecificFact). Expected impact: the single 42-minute shard should now distribute across ~5 shards of ~8-10 minutes each, reducing the critical path of TestBuild: linux (x64) by ~30 minutes.
There was a problem hiding this comment.
Pull request overview
This PR splits the previously large RunFileTests coverage into multiple smaller xUnit classes to improve Helix sharding (since the scheduler won’t split a single test class across shards), and adds a short write-up documenting the pipeline/sharding motivation.
Changes:
- Introduces
RunFileTestBaseto centralize shared helpers/test constants used across the split test classes. - Moves RunFileTests coverage into separate classes (
*_BuildOptions,*_BuildCommands,*_Directives,*_CscOnlyAndApi) to reduce shard imbalance. - Adds
documentation/general/pr-build-performance-analysis.mdexplaining the Helix shard imbalance and expected impact.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| test/dotnet.Tests/CommandTests/Run/RunFileTests_Directives.cs | Adds directive-focused tests (defines/package/sdk/project refs, include, user-secrets, CSC argument equivalence). |
| test/dotnet.Tests/CommandTests/Run/RunFileTests_CscOnlyAndApi.cs | Adds CSC-only, up-to-date, and run-api/MSBuild-get related tests. |
| test/dotnet.Tests/CommandTests/Run/RunFileTests_BuildOptions.cs | Adds tests for run/build options behavior (working dir, args passthrough, binlog/verbosity, resources). |
| test/dotnet.Tests/CommandTests/Run/RunFileTests_BuildCommands.cs | Adds tests for build-command behaviors (restore/build/publish/pack/clean/launch profiles). |
| test/dotnet.Tests/CommandTests/Run/RunFileTestBase.cs | New shared base class holding helpers/constants used by the split test classes. |
| documentation/general/pr-build-performance-analysis.md | Documents the Helix shard imbalance and the rationale/expected impact of splitting RunFileTests. |
test/dotnet.Tests/CommandTests/Run/RunFileTests_BuildOptions.cs
Outdated
Show resolved
Hide resolved
test/dotnet.Tests/CommandTests/Run/RunFileTests_CscOnlyAndApi.cs
Outdated
Show resolved
Hide resolved
test/dotnet.Tests/CommandTests/Run/RunFileTests_CscOnlyAndApi.cs
Outdated
Show resolved
Hide resolved
test/dotnet.Tests/CommandTests/Run/RunFileTests_CscOnlyAndApi.cs
Outdated
Show resolved
Hide resolved
…ildperf # Conflicts: # test/dotnet.Tests/CommandTests/Run/RunFileTests.cs
jjonescz
left a comment
jjonescz
left a comment
There was a problem hiding this comment.
Haven't verified the code changes, I assume these are just moves with no other changes.
|
|
||
| | Shard | Duration | Tests | Classes | | ||
| |---|---|---|---| | ||
| | Shard 19 | **42.1 min** | 234 | RunCommandTests, RunFileTests | |
There was a problem hiding this comment.
Should this doc file be committed given it's probably outdated now?
There was a problem hiding this comment.
I thought it was useful to save off the analysis that was done as there are additional recommendations. If you like, I can remove the data part or provide a date so we know when the data was collected.
New tests from main (Restore_NonExistentPackage) added to the appropriate split file. Changes from e4f6e1e (VirtualProjectBuilder.GetVirtualProjectPath, AssemblyName/RootNamespace in Api tests, fileName in GetGeneratedMethodName, MSBuildGet_Consistent StdErr replacement) applied to split files. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
That was the request to copilot and that's what it look like it did. There have been some conflict resolutions that were a bit more complex as the tests keep changing on me. I think they are right but I'm trusting copilot on the refactors since it's generally good with that. |
| /// Can regenerate CSC arguments template in <see cref="CSharpCompilerCommand"/>. | ||
| /// </summary> | ||
| [Fact] | ||
| public void CscArguments() |
There was a problem hiding this comment.
This and the test below would make more sense under "CscOnly" category then here in "Directives". Is it okay if I move them?
Move the CscArguments and CscVsMSBuild tests from RunFileTests_Directives to RunFileTests_CscOnlyAndApi as they are CscOnly tests, not Directive tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
I was also looking at the failures but something else came up. Thanks, this fix looks good. |
See the .md file included in this PR. I asked copilot to do an analysis of our PR builds to look for easy wins. This seemed like the lowest risk change to initially make to improve some of the PR times. Splitting the test classes by time would be better but that's more complicated to implement. I'll see if there are other recommendations that are low hanging fruit to attempt.
Problem:
RunFileTestswas a single class with ~150 IL methods. The scheduler placed it entirely in shard 19 alongsideRunCommandTests(4 methods), creating a 42-minute shard while all other dotnet.Tests.dll shards completed in under 8 minutes.